Entity type provided by a module installed via update hook not available #4103

claudiu-cristea · 2019-06-24T16:10:34Z

If an entity type is provided by a module being installed via a (post)update hook. The next Drush command will fail to recognize the new installed definition, unless a cache rebuild is run just after updatedb. For example, with this hook:

function woot_post_update_install_taxonomy()
{
    \Drupal::service('module_installer')->install(['taxonomy']);
    return \Drupal::entityTypeManager()->getDefinition('taxonomy_term')->id();
}

When running:

$ drush updb --yes

This will end successfully and will return taxonomy_term, but if I run immediately:

$ drush eval 'print \Drupal::entityTypeManager()->getDefinition("taxonomy_term")->id();'

...will output:

In EntityTypeManager.php line 150:

  The "taxonomy_term" entity type does not exist.

The bug is reproduced in the provided regression test. Rebuilding the cache in between is a workaround. This was not a bug before. We see this happening after moving to Drupal 8.7.x (is it probably related to https://www.drupal.org/node/3034742?)

It's interesting that the post-update recognizes the new installed entity type, as it returns its ID. This is not a Drupal issue. I tested the same scenario by running the UI update and works without error. I also run the next sequence with no errors:

$ drush en taxonomy
$ drush eval 'print \Drupal::entityTypeManager()->getDefinition("taxonomy_term")->id();'

The issue pops-up only when enabling the module via a (post)update.

pfrenssen

Thanks for the fix and the test coverage! Code looks good and I can replicate the failures.

I have added some documentation suggestions.

src/Commands/core/UpdateDBCommands.php

tests/functional/resources/modules/d8/woot/woot.module

pfrenssen · 2019-06-28T13:33:51Z

Looks good to me, thanks for the contribution!

weitzman · 2019-06-30T03:02:41Z

Lets hold off on merging this. I want to get an old Pr linked here which touched on this issue, or a similar one.

claudiu-cristea · 2019-06-30T10:31:34Z

Lets hold off on merging this. I want to get an old Pr linked here which touched on this issue, or a similar one.

@weitzman

I can't find the source of this bug, it might not be a regression. It's very possible that this didn't surfaced. But the proof is there, in the test.

weitzman · 2019-06-30T11:44:59Z

The issue I was thinking of is #2617 and its PR is #2620. I just mention them in case anyone finds them relevant.

If an entity type is provided by a module being installed via a (post)update hook

Is it really acceptable to install modules in a post_update hook? I would think that you enable the modules you want to enable locally, and deploy a new core.extension.yml to the higher environments. Perhaps there are other use cases.

Also, woot_post_update_install_taxonomy() does't run batches like pm:enable does. See

drush/src/Drupal/Commands/pm/PmCommands.php

Lines 78 to 83 in 40092d5

    
           if (!$this->getModuleInstaller()->install($modules, true)) { 
        
               throw new \Exception('Unable to install modules.'); 
        
           } 
        
           if (batch_get()) { 
        
               drush_backend_batch_process(); 
        
           }

. Maybe that would fix the missing definition issue?

Finally, why does the cache-rebuild not fix this -

drush/src/Commands/core/UpdateDBCommands.php

Lines 425 to 426 in d17e067

    
           // Only needed if we performed updates earlier. 
        
           $operations[] = ['\Drush\Commands\core\UpdateDBCommands::cacheRebuild', []];

?

pfrenssen · 2019-07-01T14:02:38Z

Is it really acceptable to install modules in a post_update hook? I would think that you enable the modules you want to enable locally, and deploy a new core.extension.yml to the higher environments. Perhaps there are other use cases.

We are doing this when we are adding a new module dependency to an existing custom module that is optional in our distribution. It is possible users are running a different set of modules than we originally ship the distribution with. We don't control their config but we need to ensure somehow that they will have the module enabled after they update to our latest version. Maybe it is possible to do this in pure config, but I don't know how.

pfrenssen · 2019-07-01T15:03:21Z

Also, woot_post_update_install_taxonomy() does't run batches like pm:enable does

This is a very interesting suggestion. I investigated this. This section to run the batch processes was added in #3444 to ensure that the translation imports are completed when the Locale module is enabled.

Indeed, the Locale module sets a batch in locale_system_update(), but nowhere in the ModuleInstaller or related code is a batch process launched. I tracked it down to FormSubmitter::doSubmitForm(). Basically any form is allowed to set a batch in the submit handler, and this will be executed on form submit.

So this means that somehow the Locale module is fine with translations only being imported when the module is enabled as part of a form submit. I could not find any other modules that attempt this, and it doesn't seem likely that the rebuilding of the container is handled in this way. Most probably in the "normal" install procedure (which is ModulesListForm::submitForm()) this just happens to work because the request ends after the modules are installed, and there is no more code running that needs the container to do entity operations.

I also wanted to understand how core handles the mangled container when performing database updates, since this evidently works fine in core but somehow not in Drush. It turns out that a container rebuild is triggered automatically as part of the update procedure. This is initiated through the call to drupal_flush_all_caches() in DbUpdateController::batchFinished(). As part of flushing the caches a call is done to \Drupal::service('kernel')->invalidateContainer().

So I am thinking that it is probably a good idea to copy this behavior to UpdateDbCommands::updateFinished(). Let's just do an unconditional call to drupal_flush_all_caches(), without checking if any extensions have changed. This will most closely mimic the actual behavior from core, and will probably solve some other edge cases that people might be experiencing.

pfrenssen · 2019-07-01T15:04:11Z

src/Commands/core/UpdateDBCommands.php

+     * The list of extensions that are installed before updates are applied.
+     * @var array
+     */
+    protected $originalExtensionList;


Let's remove this section, we no longer need to track if extensions are updated.

pfrenssen · 2019-07-01T15:07:13Z

src/Commands/core/UpdateDBCommands.php

+        $final_extension_list = \Drupal::config('core.extension')->getRawData();
+        if ($final_extension_list !== $this->originalExtensionList) {
+            // Invalidate the container if the installed extension list has been changed.
+            \Drupal::service('kernel')->invalidateContainer();


Let's replace this with a simple cache flush:

public static function updateFinished($success, $results, $operations) { // Flush all caches. Drupal core does the same after finishing the updates. // @see \Drupal\system\Controller\DbUpdateController::batchFinished() drupal_flush_all_caches(); }

pfrenssen · 2019-07-01T15:07:41Z

src/Commands/core/UpdateDBCommands.php

    /**
     * Start the database update batch process.
     * @param $options
     * @return bool
     */
    public function updateBatch($options)
    {
+        // Track the currently installed extensions so that we can detect if any
+        // new extensions are added in the update.
+        $this->originalExtensionList = \Drupal::config('core.extension')->getRawData();


We do not need this any more.

weitzman · 2019-07-01T15:18:44Z

So I am thinking that it is probably a good idea to copy this behavior to UpdateDbCommands::updateFinished(). Let's just do an unconditional call to drupal_flush_all_caches(), without checking if any extensions have changed.

That makes sense to me. We've had that clear and removed it a couple times over the long life of udpatedb. Please add code comments so we dont remove it again. Also, lets review the cache clears already in the command to see if any can now be removed.

claudiu-cristea · 2019-07-02T10:48:23Z

@weitzman @pfrenssen I cannot explain the failure with Windows, it seems unrelated for me and I don't know how to trigger a rebuild.

src/Commands/core/UpdateDBCommands.php

pfrenssen · 2019-07-02T11:39:30Z

The AppVeyor failure seems to be unrelated to this issue, I see that the master is affected by the same problem:https://github.com/drush-ops/drush/commits/master

pfrenssen · 2019-07-03T06:41:17Z

Great, looking good to me, thanks!

pfrenssen · 2019-07-03T06:49:07Z

Also, lets review the cache clears already in the command to see if any can now be removed.

I looked into this. There is one other instance where the cache is cleared, in ::cacheRebuild() but this is still needed. This performs a cache clear in between the regular updates and the post-updates, and is also matching the behavior of core. The relevant core code can be found in \Drupal\system\Controller\DbUpdateController::triggerBatch():

    if ($post_updates) {
      // Now we rebuild all caches and after that execute the hook_post_update()
      // functions.
      $operations[] = ['drupal_flush_all_caches', []];
      foreach ($post_updates as $function) {
        $operations[] = ['update_invoke_post_update', [$function]];
      }
    }

weitzman · 2019-07-17T11:26:59Z

src/Commands/core/UpdateDBCommands.php

+        // performs database updates it also clears the cache at the end. This
+        // ensures that we are compatible with updates that rely on this
+        // behavior.
+        drupal_flush_all_caches();


Should only run if the option --cache-clear is true. PR can be merged after thats done.

…ilable. Regression test.

Co-Authored-By: Pieter Frenssen <pieter@frenssen.be>

…dates.

claudiu-cristea · 2019-07-23T08:20:45Z

@weitzman I've added the check. I had to make updateFinished() non-static otherwise it would make to complicated to pass the option.

pfrenssen self-assigned this Jun 28, 2019

pfrenssen added area-updatedb type-bug labels Jun 28, 2019

pfrenssen reviewed Jun 28, 2019

View reviewed changes

src/Commands/core/UpdateDBCommands.php Outdated Show resolved Hide resolved

src/Commands/core/UpdateDBCommands.php Outdated Show resolved Hide resolved

tests/functional/resources/modules/d8/woot/woot.module Outdated Show resolved Hide resolved

pfrenssen removed their assignment Jun 28, 2019

pfrenssen approved these changes Jun 28, 2019

View reviewed changes

pfrenssen added the stat-ready-to-merge label Jun 28, 2019

pfrenssen removed the stat-ready-to-merge label Jul 1, 2019

pfrenssen requested changes Jul 1, 2019

View reviewed changes

pfrenssen reviewed Jul 2, 2019

View reviewed changes

src/Commands/core/UpdateDBCommands.php Outdated Show resolved Hide resolved

pfrenssen approved these changes Jul 3, 2019

View reviewed changes

pfrenssen added the stat-ready-to-merge label Jul 3, 2019

weitzman approved these changes Jul 17, 2019

View reviewed changes

claudiu-cristea added 6 commits July 23, 2019 10:07

An entity type provided by a module installed via update hook not ava…

7c78b07

…ilable. Regression test.

Invalidate the container if the installed extensions has changed.

4ebda17

Enable taxonomy instead of entity_test to avoid all other tests failure.

dbadaf6

Move the container invalidation in the batch 'finished' callback.

f7d6826

On Windows platforms the php:eval argument gets backslash escaped twice.

e901a6d

Fix docs.

e1810a7

claudiu-cristea and others added 6 commits July 23, 2019 10:07

Add docs for UpdateDBCommands::originalExtensionList.

691c365

Co-Authored-By: Pieter Frenssen <pieter@frenssen.be>

Document why we're getting the original list of installed extensions.

605a34e

Co-Authored-By: Pieter Frenssen <pieter@frenssen.be>

Improve the documentation of woot helper function.

4c36606

Co-Authored-By: Pieter Frenssen <pieter@frenssen.be>

Unconditionally clear the cache and invalidate the container after up…

e6da5ab

…dates.

Fix updateFinished() documentation.

e70f7a2

Only flush caches if --cache-clear.

29f0fc3

weitzman merged commit 4f35662 into drush-ops:master Jul 23, 2019

Berdir mentioned this pull request Jul 31, 2020

drush updb does not clear caches if there are no post updates #4494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity type provided by a module installed via update hook not available #4103

Entity type provided by a module installed via update hook not available #4103

claudiu-cristea commented Jun 24, 2019 •

edited

Loading

pfrenssen left a comment

pfrenssen commented Jun 28, 2019

weitzman commented Jun 30, 2019

claudiu-cristea commented Jun 30, 2019 •

edited

Loading

weitzman commented Jun 30, 2019 •

edited

Loading

pfrenssen commented Jul 1, 2019 •

edited

Loading

pfrenssen commented Jul 1, 2019 •

edited

Loading

pfrenssen Jul 1, 2019

pfrenssen Jul 1, 2019

pfrenssen Jul 1, 2019

weitzman commented Jul 1, 2019

claudiu-cristea commented Jul 2, 2019

pfrenssen commented Jul 2, 2019

pfrenssen commented Jul 3, 2019

pfrenssen commented Jul 3, 2019

weitzman Jul 17, 2019

claudiu-cristea commented Jul 23, 2019

Entity type provided by a module installed via update hook not available #4103

Entity type provided by a module installed via update hook not available #4103

Conversation

claudiu-cristea commented Jun 24, 2019 • edited Loading

pfrenssen left a comment

Choose a reason for hiding this comment

pfrenssen commented Jun 28, 2019

weitzman commented Jun 30, 2019

claudiu-cristea commented Jun 30, 2019 • edited Loading

weitzman commented Jun 30, 2019 • edited Loading

pfrenssen commented Jul 1, 2019 • edited Loading

pfrenssen commented Jul 1, 2019 • edited Loading

pfrenssen Jul 1, 2019

Choose a reason for hiding this comment

pfrenssen Jul 1, 2019

Choose a reason for hiding this comment

pfrenssen Jul 1, 2019

Choose a reason for hiding this comment

weitzman commented Jul 1, 2019

claudiu-cristea commented Jul 2, 2019

pfrenssen commented Jul 2, 2019

pfrenssen commented Jul 3, 2019

pfrenssen commented Jul 3, 2019

weitzman Jul 17, 2019

Choose a reason for hiding this comment

claudiu-cristea commented Jul 23, 2019

claudiu-cristea commented Jun 24, 2019 •

edited

Loading

claudiu-cristea commented Jun 30, 2019 •

edited

Loading

weitzman commented Jun 30, 2019 •

edited

Loading

pfrenssen commented Jul 1, 2019 •

edited

Loading

pfrenssen commented Jul 1, 2019 •

edited

Loading